Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

detect key from longer URL #1175

Merged
merged 28 commits into from
Oct 30, 2022
Merged

detect key from longer URL #1175

merged 28 commits into from
Oct 30, 2022

Conversation

vanithaak
Copy link
Contributor

@vanithaak vanithaak commented Oct 16, 2022 •
edited
Loading

Fixes #998 (<=== Add issue number here)

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

  • PR is descriptively titled 📑 and links the original issue above 🔗
  • tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt test
  • code is in uniquely-named feature branch and has no merge conflicts 📁
  • screenshots/GIFs are attached 📎 in case of UI updates
  • @mention the original creator of the issue in a comment below for help or for a review

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

Thanks!

GIF

MapKnitter.Lite.-.Google.Chrome.2022-10-21.16-45-53.1.mp4

@gitpod-io
Copy link

gitpod-io bot commented Oct 16, 2022

@vanithaak
Copy link
Contributor Author

vanithaak commented Oct 16, 2022 •
edited
Loading

@jywarren please review my pr, Thanks!!

@jywarren
Copy link
Member

Hi, thanks for taking this up! However I think it needs a little more detail in the original problem statement. What I was thinking was, people may input either a full URL like https://archive.org/details/texas-barnraising, or they might put just the key like texas-barnraising -- they might even put just archive.org/details/texas-barnraising? Could we make a function which would output texas-barnraising in any of these three cases, or any other combinations you can think of?

I hope that helps! You've made a good start here!

@vanithaak
Copy link
Contributor Author

vanithaak commented Oct 21, 2022 •
edited
Loading

@TildaDares , @jywarren please review my pr, Thanks!

jywarren
jywarren reviewed Oct 23, 2022
View reviewed changes
examples/archive.html
@@ -94,7 +94,29 @@ <h3 id="offcanvasRightLabel">Images</h3>

form.addEventListener('submit', (event) => {
event.preventDefault();
const url = input.value.replace('details', 'metadata');
let getUrl;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good! I'd like to test it out a bit. But can you put it within its own named function, like function showImages(getUrl) { below? Maybe it could be named function extractKey()?

@vanithaak
Copy link
Contributor Author

@jywarren , please review my pr

@vanithaak vanithaak mentioned this pull request Oct 24, 2022
Closed
4 tasks
@vanithaak
Copy link
Contributor Author

@jywarren , please review my pr. Thanks!

@jywarren jywarren closed this Oct 30, 2022
@jywarren jywarren reopened this Oct 30, 2022
@jywarren
Copy link
Member

Oops, sorry, i accidentally pressed the wrong button!

Very nice. I tested it, and several variations worked fine. However, I noticed that entering this did not work:

http://archive.org/details/texas-barnraising

I'm going to merge this, but maybe we can add an additional line to add the s if it's just http://?

@jywarren
Copy link
Member

I'm going to merge this, but maybe we can add an additional line to add the s if it's just http://?

Actually maybe that could be a first-timers-only issue?

@jywarren jywarren merged commit ff73b8d into publiclab:main Oct 30, 2022
@jywarren
Copy link
Member

Nicely done!!!

@vanithaak
Copy link
Contributor Author

vanithaak commented Oct 31, 2022 •
edited
Loading

I'm going to merge this, but maybe we can add an additional line to add the s if it's just http://?

Actually maybe that could be a first-timers-only issue?

Hi @jywarren, would you like me to make a first-timer-issue?

@segun-codes
Copy link
Collaborator

I'm going to merge this, but maybe we can add an additional line to add the s if it's just http://?

Actually maybe that could be a first-timers-only issue?

Hi @jywarren, would you like me to make a first-timer-issue?

Hi @vanithaak, since in the conversation here https://github.com/publiclab/Leaflet.DistortableImage/pull/1232 @jywarren suggested I could fix the http/https issue, so I would simply attend to it and that will be it.

@vanithaak
Copy link
Contributor Author

Hi @vanithaak, since in the conversation here https://github.com/publiclab/Leaflet.DistortableImage/pull/1232 @jywarren suggested I could fix the http/https issue, so I would simply attend to it and that will be it.

Hey ! sounds good! I'll create an issue for you to contribute!

@vanithaak vanithaak mentioned this pull request Oct 31, 2022
Closed
vanithaak added a commit to vanithaak/Leaflet.DistortableImage that referenced this pull request Nov 5, 2022
@vanithaak @7malikk @jywarren
detect key from longer URL (publiclab#1175)
84062f7 
* detect key from longer URL

* detect key from longer URL

Co-authored-by: 7malikk <7malikk@gmail.com>

* fixes

* made a more flexible function to handle url

* updates

* made a more flexible function to handle url

* made new function extractKey

* replace 'http:' to https:

* updated http functionality

Co-authored-by: 7malikk <7malikk@gmail.com>
Co-authored-by: Jeffrey Warren <jeff@unterbahn.com>
@7malikk 7malikk mentioned this pull request Dec 7, 2022
Open
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jywarren jywarren jywarren left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MapKnitter Lite: fetch images from Internet Archive
3 participants
@vanithaak @jywarren @segun-codes